Skip to content

Conversation

@jamesjscully
Copy link
Contributor

No description provided.

@ChrisRackauckas
Copy link
Member

I guess this is fine. It may not be the most useful though since lowering depends on context, and if it's no context then people might want simplified_expr instead.

@codecov
Copy link

codecov bot commented Oct 3, 2019

Codecov Report

Merging #191 into master will decrease coverage by 0.18%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #191      +/-   ##
==========================================
- Coverage   92.75%   92.56%   -0.19%     
==========================================
  Files          14       14              
  Lines         469      471       +2     
==========================================
+ Hits          435      436       +1     
- Misses         34       35       +1
Impacted Files Coverage Δ
src/operations.jl 68.75% <100%> (-2.68%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 870f80f...3bf6b6c. Read the comment docs.

@jamesjscully
Copy link
Contributor Author

aside from changing variables like y(t) to y, this is a subset. I didn't know simplify_expr existed. simplify_expr is essentially what i was going for. Looking at it, this one doesn't handle constants or differentials correctly. When would it be useful to have y(t) in the expression instead of y though?eval(:(y(t)))#error

@jamesjscully
Copy link
Contributor Author

nevermind just saw you already explained this in your comment 🥇

@ChrisRackauckas
Copy link
Member

When would it be useful to have y(t) in the expression instead of y though?eval(:(y(t)))#error

When it's actually a function, for example time-varying covariate, or the solution to a differential equation.

So should this just be replaced with simplify_expr? That might make it easier for most use cases.

@jamesjscully
Copy link
Contributor Author

Yes simplify_expr is probably better

@ChrisRackauckas ChrisRackauckas merged commit 3bf6b6c into SciML:master Oct 5, 2019
@ChrisRackauckas
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants